Transport completions #2524
Transport completions #2524
Conversation
yedayak
force-pushed
the
transport-completions
branch
from
5e6d14c to
627aced
Compare
yedayak
force-pushed
the
transport-completions
branch
from
627aced to
580ed9f
Compare
Signed-off-by: Yedaya Katsman <yedaya.ka@gmail.com>
yedayak
force-pushed
the
transport-completions
branch
2 times, most recently
from
1539aa9 to
8f37068
Compare
cmd/skopeo/completions.go
Outdated
| func autocompleteTransports(cmd *cobra.Command, args []string, toComplete string) ([]cobra.Completion, cobra.ShellCompDirective) { | ||
| directive := cobra.ShellCompDirectiveNoSpace | ||
| // We still don't have a transport | ||
| if !strings.Contains(toComplete, ":") { |
There was a problem hiding this comment.
Overall this would likely be much better written with strings.Cut()
That way you can easily check if a colon was set and the you have the clear prefix and can match that without the colon below.
There was a problem hiding this comment.
Yeah, this works much better. Updated to do this
cmd/skopeo/completions.go
Outdated
| if strings.HasPrefix(toComplete, "oci-archive:") || strings.HasPrefix(toComplete, "docker-archive:") || strings.HasPrefix(toComplete, "sif:") || strings.HasPrefix(toComplete, "oci:") { | ||
| return nil, directive | ||
| } | ||
| if toComplete == "docker:" { | ||
| return []cobra.Completion{"docker://"}, directive | cobra.ShellCompDirectiveNoFileComp | ||
| } | ||
| if strings.HasPrefix(toComplete, "dir:") { |
There was a problem hiding this comment.
here you then should match the transpart names as defined by the code and not hard code the strings
docker.Transport.Name(), directory.Transport.Name(),etc...
cmd/skopeo/completions.go
Outdated
| // just ShellCompDirectiveFilterDirs is more correct, but doesn't work here in bash, see https://github.com/spf13/cobra/issues/2242. Instead we get the directories ourselves. | ||
| curDir := filepath.Dir(strings.TrimPrefix(toComplete, "dir:")) | ||
| entries, err := os.ReadDir(curDir) | ||
| if err != nil { | ||
| cobra.CompErrorln("Failed ReadDir at " + curDir) | ||
| // Fallback to whatever the shell gives us | ||
| return nil, cobra.ShellCompDirectiveFilterDirs | cobra.ShellCompDirectiveNoFileComp | ||
| } | ||
| suggestions := make([]cobra.Completion, 0, len(entries)) | ||
| for _, e := range entries { | ||
| if e.IsDir() { | ||
| suggestions = append(suggestions, "dir:"+path.Join(curDir, e.Name())+"/") | ||
| } | ||
| } | ||
| return suggestions, cobra.ShellCompDirectiveNoFileComp | cobra.ShellCompDirectiveNoSpace |
There was a problem hiding this comment.
Is that actually worth it? Returning all paths as done by the shell seems more than fine, compared of having to read all files here.
There was a problem hiding this comment.
I agree its a bit overkill, this just makes the completions more accurate, showing only directories. I'm fine with not doing it.
yedayak
force-pushed
the
transport-completions
branch
2 times, most recently
from
1f10c79 to
0dcebd9
Compare
|
Tests failed. @containers/packit-build please check. |
|
@Luap99 do we test completions in CI anywhere yet? |
In podman we have number of rather complex tests (test/system/600-completion.bats). But that is not something I would recommend for skopeo, most of the changes here are trivial to test manually (and it is not like they will get changed often) and there is basically no risk if the completion is not working fully. |
Here, I think it would be easier, and we would not give up on ~any test coverage compared to what Podman does, to use Go unit tests for the individual parts (and, for that purpose, to split the
Yes. I don’t think missing tests are a blocker. |
Transports that reference a file or directory are completed. Signed-off-by: Yedaya Katsman <yedaya.ka@gmail.com>
yedayak
force-pushed
the
transport-completions
branch
from
0dcebd9 to
d577111
Compare
| suggestions := make([]cobra.Completion, 0, len(entries)) | ||
| for _, e := range entries { | ||
| if e.IsDir() { | ||
| suggestions = append(suggestions, transport+":"+path.Join(curDir, e.Name())+"/") |
There was a problem hiding this comment.
This should use path/filepath
| } | ||
| return suggestions, cobra.ShellCompDirectiveNoFileComp | cobra.ShellCompDirectiveNoSpace | ||
| } | ||
| if transport == docker.Transport.Name() && details == "" { |
There was a problem hiding this comment.
Non-blocking: I think this can also be within the switch transport (with the if separate).
Another thing I would like to add is completion of
containers-storage:, but I'm not sure how to get the list of available images.